-
Notifications
You must be signed in to change notification settings - Fork 13.8k
resolve: Do not finalize shadowed bindings #145113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Looks like this triggers a new unused warning in the stdlib? Is this intentional? We probably should crater this if so. @bors2 try |
Reminder, once the PR becomes ready for a review, use |
resolve: Do not finalize shadowed bindings
Yes, some new unused items can be detected now. |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
5feb92e
to
a4b67f7
Compare
A few new unused import lints, as expected. |
This comment has been minimized.
This comment has been minimized.
|
712cc8f
to
5380fe6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
5380fe6
to
581051e
Compare
ping @compiler-errors |
Is there any way to write a test for this? The effect of this that we expect for users is just some new unused import warnings, right? |
Ah, the test for preludes was already merged as a part of #144793 ( The removed unused imports across the repo are mostly like this: #[macro_export]
macro_rules! mac { () => {} }
fn main() {
use crate::mac; // unused, `mac` as `macro_rules!` is already in scope and has higher priority
mac!();
}
Yes. |
This comment was marked as resolved.
This comment was marked as resolved.
r? compiler |
r? compiler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add tests that show the change in behavior wrt to lints
i think i understand what this is doing and it looks good to me
581051e
to
81acd32
Compare
This comment has been minimized.
This comment has been minimized.
I've added #145113 (comment) to test cases in addition to |
This comment has been minimized.
This comment has been minimized.
I.e. do not mark them as used, or non-speculative loaded, or similar. Previously they were sometimes finalized during early resolution, causing issues like rust-lang#144793 (comment).
81acd32
to
f89660e
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
I.e. do not mark them as used, or non-speculatively loaded, or similar.
Previously they were sometimes finalized during early resolution, causing issues like #144793 (comment).